Enforce repair contract on the final repaired tree#341
Conversation
|
Codex review: needs maintainer review before merge. Reviewed July 4, 2026, 5:13 AM ET / 09:13 UTC. Summary Reproducibility: yes. Current main has no Review metrics: 2 noteworthy metrics.
Root-cause cluster Members:
Proposal only: this assessment does not dispatch repair, suppress jobs, mutate sibling items, close, or merge anything. Merge readiness Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch. Risk before merge
Maintainer options:
Next step before merge
Security Review detailsBest possible solution: Land this after exact-head CI and maintainer automation review confirm the schema rollout and recovery-push timing are acceptable, keeping Do we have a high-confidence way to reproduce the issue? Yes. Current main has no Is this the best way to solve the issue? Yes. The current PR is a narrow maintainable path: it keeps the contract explicit, enforces it once on the final tree, makes null the compatible default, and adds regression coverage for the previous schema blocker. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against 19ca145ed965. Label changesLabel changes:
Label justifications:
Evidence reviewedWhat I checked:
Likely related people:
What the crustacean ranks mean
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics. How this review workflow works
|
05484c7 to
da96bec
Compare
|
Updated this PR after the maintainer deep review on #340. Changes now address the two blockers called out there: Validation run locally: All passed. @clawsweeper re-review |
|
🦞🧹 I asked ClawSweeper to review this item again. |
v3 update pushedI updated this PR to address the maintainer blockers from #340 instead of reopening or duplicating that closed PR. What changed: Validation run locally: Result: Local dry-run executor proof with canonical |
|
@clawsweeper re-review Please review the current head |
|
Small implementation-shape note for review: if maintainers prefer a narrower contract activation policy, I can make that adjustment in this PR without changing the overall design. The intentionally small knobs are: One extra architectural motivation for this shape: keeping That keeps the core provider-neutral: ClawSweeper owns schema, validation, git/checkpoint safety, and PR mechanics, while any external worker that can emit the canonical artifact can interoperate through the same contract. This should make the repair lane easier to extend safely without growing backend-specific logic in the core. Those would be narrow follow-ups on the current head, not a new PR and not a redesign of the checkpoint gate. |
|
Status refresh for maintainers:
Ready for maintainer review. If the preferred merge shape is narrower, I can adjust the activation policy without changing the core contract. |
486a5e0 to
bae964f
Compare
|
@clawsweeper re-review Please review exact head |
|
🦞🧹 I asked ClawSweeper to review this item again. |
|
@clawsweeper re-review The strict review schema outage is fixed on |
|
Landed in d3ad07b. The final rewrite keeps the contributor's final-tree repair contract, makes Proof on exact head
Thank you @Jhacarreiro for the original repair-contract proposal, and @vincentkoc for the rebase/checkpoint follow-up work. |
What Problem This Solves
Repair artifacts can name files that must be present in the finished repair. The prior implementation enforced that contract at every intermediate checkpoint, so a valid initial repair could be rejected when a later review fix, base sync, or finalize step touched only an adjacent test or conflict file.
It could also publish recovery checkpoints before the whole repaired tree had been validated.
Why This Change Was Made
This keeps the contributor's canonical
fix_artifact.repair_contractboundary, but makes the invariant describe the final product instead of each implementation step:must_touch: required file or directory pathsmatch:anyorallscopeThe executor validates the final
origin/<base>..HEADchanged-file set after review fixes, final base synchronization, and replacement-history compaction. That latest-base comparison prevents an upstream-only change from satisfying the contract.Contract-bearing replacement jobs defer recovery pushes until this final validation passes. Jobs without a repair contract keep their existing checkpoint recovery behavior. Pure deterministic rebases cannot carry a repair contract.
User Impact
Evidence
Exact reviewed head:
97f4ff5ebeb3c225b55bac561716e76617484630The integration test creates a real temporary Git repository and proves both sides of the contract:
Fresh full-branch autoreview after the final implementation reported no accepted/actionable findings:
patch is correct(0.82 confidence).This is a maintainer rewrite of the contributor's original repair-contract proposal. The original commits and authors remain in the PR history.